Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some warnings #984

Open
wants to merge 1 commit into
base: vanilla
Choose a base branch
from
Open

Conversation

th-otto
Copy link
Contributor

@th-otto th-otto commented Mar 24, 2024

No description provided.

@th-otto th-otto force-pushed the PR-37 branch 2 times, most recently from bc5661c to fc42152 Compare March 24, 2024 09:55
Copy link
Collaborator

@giulianobelinassi giulianobelinassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at my comments.

@@ -107,7 +107,7 @@ class GenericNode
}
bool Is_Valid(void) const
{
return (this != NULL && NextNode != NULL && PrevNode != NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually seems to be something intended by the original programmers rather than a programming error. Are you sure this is actually not necessary?

For example, if you call ->Is_Valid() with a nullptr object this function will return false rather than crashing the code.

@@ -131,8 +131,12 @@ PacketClass::PacketClass(char* curbuf)
//
// Copy the adjusted header into the buffer and then advance the buffer
//
memcpy(field, curbuf, FIELD_HEADER_SIZE);
curbuf += FIELD_HEADER_SIZE;
memcpy(field->ID, curbuf, 4);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should take the sizeof(field->ID) rather than hardcoding it to 4. If we decide later to change this in the future this memcpy would be very error prone.

@@ -118,7 +118,8 @@ PaletteClass& PaletteClass::operator=(PaletteClass const& palette)
if (this == &palette)
return (*this);

memcpy(&Palette[0], &palette.Palette[0], sizeof(Palette));
for (int i = 0; i < PaletteClass::COLOR_COUNT; i++)
Palette[i] = palette.Palette[i];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replacing a very optimized copy of palettes with a potentially non-optimized one. Are you sure this is necessary?

@@ -108,7 +108,9 @@ void RandomStraw::Reset(void)
{
SeedBits = 0;
Current = 0;
memset(Random, '\0', sizeof(Random));
for (size_t i = 0; i < sizeof(Random) / sizeof(Random[0]); i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ARRAY_SIZE, defined in common/macros.h

@@ -210,7 +212,7 @@ void RandomStraw::Seed_Byte(char seed)
*=============================================================================================*/
void RandomStraw::Seed_Short(short seed)
{
for (int index = 0; index < (sizeof(seed) * CHAR_BIT); index++) {
for (int index = 0; index < ((int)sizeof(seed) * CHAR_BIT); index++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t may be cleaner than int here

@@ -250,7 +250,7 @@ inline static void _splitpath(const char* path, char* drive, char* dir, char* fn
return;
}

for (int i = 0; i < strlen(path); i++) {
for (int i = 0; i < (int)strlen(path); i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid calling strlen in a loop comparison like this. This is O(n²) instead of O(n). Instead store the size of the string into a variable and use that instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants